Skip to content

Conversation

@vksir
Copy link
Contributor

@vksir vksir commented Nov 6, 2025

What does this PR try to resolve?

When executing the cargo vendor command, there is a probability that mv pkg fails, causing strip_prefix to panic. This PR aims to fix this issue.

Fix: #15875

How to test and review this PR?

Modify the code to make the move pkg action inevitably fail. Then rebuild cargo and execute cargo vendor to test it.

@rustbot rustbot added Command-vendor S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 6, 2025
@rustbot
Copy link
Collaborator

rustbot commented Nov 6, 2025

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@vksir vksir force-pushed the master branch 4 times, most recently from fba40a8 to b751696 Compare November 6, 2025 14:57
Comment on lines 236 to 238
// Comment it to keep the blocking file to force rename failure.
// This action Will be undo later.
// let _ = fs::remove_dir_all(&dst);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to comment out this to reproduce?

As documented, remove_dir_all will fail if the path is not a directory, so our fs::rename call should fail even without commenting this out I guess.

(And we also capture CARGO_LOG in the stderr assertion, so it should be fairly robust to ensure we went the code path)

Copy link
Contributor Author

@vksir vksir Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use std::fs;

fn main() {
    fs::create_dir_all("./tmp").unwrap();
    fs::create_dir_all("./tmp/src").unwrap();
    fs::write("./tmp/src/src_file", "src_file").unwrap();

    // Will rename successfully
    fs::write("./tmp/dst", "blocking file").unwrap();
    fs::rename("./tmp/src", "./tmp/dst").unwrap();
}

I wrote a smallest test. fs::rename will make the src dir override the dst file. So I change the dst file to a dir in pre commit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote a smallest test. fs::rename will make the src dir override the dst file. So I change the dst file to a dir in pre commit.

Interesting 🤔. On Linux it failed: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=0b8b476b80e4bc4ac6839433e39fd840

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I just thought of this as well and tested it. It failed in the Linux environment. (In the windows, it will rename successfully).
What ever, it seems that the unit test is running on the Linux docker. I would modify the code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/rust-lang/rust/blob/51f5892019f8fb07864647d46c4eb577d3b0719f/library/std/src/sys/fs/windows.rs?plain=1#L1272

Found it. The Windows implementation of fs::rename uses MOVEFILE_REPLACE_EXISTING. Probably that was the reason it succeeded?
Anyway, if we cannot make it passed on Windows, we can put this

#[cargo_test(ignore_windows = "annoying to create a repro on Windows; see rust-lang/cargo#16214")]

@vksir vksir force-pushed the master branch 6 times, most recently from 22f0d39 to 14310d5 Compare November 6, 2025 19:00
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

---- expected: tests/testsuite/vendor.rs:2117:27
++++ actual:   stderr
   1    1 | ...
   2    2 | [..]failed to `mv "[..]vendor[..].vendor-staging[..]log-0.3.5" "[..]vendor[..]log"`: [..]
   3    3 | ...
   4    4 | Caused by:
   5    5 |   failed to copy vendored sources for log v0.3.5
   6    6 | 
   7    7 | Caused by:
   8      -   failed to create directory `[..]log`
        8 +   failed to create directory `[ROOT]/foo/vendor/log/src`
   9    9 | 
  10   10 | Caused by:
  11      -   File exists[..]
       11 +   Not a directory (os error 20)
  12   12 | ...∅

Ouch! Then we have no way out for this.

maybe we should have a private environment variable unconditionally go through that code path? Like __CARGO_TEST_VENDOR_FALLBACK_CP_SOURCES"? So that we can actually verify the cp_sources is working correct. And with that we don't need to ignore Windows anymore.

View changes since this review

@vksir
Copy link
Contributor Author

vksir commented Nov 7, 2025

Okay! If adding environment variables is allowed, I'm glad to do so.

@weihanglo
Copy link
Member

@vksir yeah go head. Just remind that we disallow std::env::var use throughout Cargo, so you may either put an allowed like this

// ALLOWED: For testing Cargo itself only.
#[allow(clippy::disallowed_methods)]

or access env from gctx

if gctx.get_env_os("__CARGO_TEST_INTERNAL_ERROR").is_some() {

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thanks for working with us!

View changes since this review

@weihanglo weihanglo added this pull request to the merge queue Nov 10, 2025
Merged via the queue into rust-lang:master with commit af465ec Nov 10, 2025
25 checks passed
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 10, 2025
@vksir
Copy link
Contributor Author

vksir commented Nov 11, 2025

@weihanglo Also thank you once again for your patient guidance! :)

bors added a commit to rust-lang/rust that referenced this pull request Nov 12, 2025
Update cargo

10 commits in 445fe4a68f469bf936b2fd81de2c503b233a7f4f..2d4fa139552ebdd5f091a1401ed03f7dc62cb43f
2025-11-07 18:08:19 +0000 to 2025-11-12 15:56:06 +0000
- feat: Add unstable rustc-unicode flag (rust-lang/cargo#16243)
- fix(package): all tar entries timestamp be the same (rust-lang/cargo#16242)
- feat: emit help messages for github pull request url in dependency (rust-lang/cargo#16207)
- docs: fix comments for alternative registry fns (rust-lang/cargo#16235)
- add into_value utility function for inheritableField (rust-lang/cargo#16234)
- fix(command-vendor): strip_prefix panic in cp_sources method (rust-lang/cargo#16214)
- fix(lock): Be moore direct in the error message (rust-lang/cargo#16233)
- fix(lock): In error, differentiate between creating and updating lockfile (rust-lang/cargo#16227)
- fix(cli): Refer to commands, not subcommands (rust-lang/cargo#16226)
- fix(run): Help teach about argument escaping (rust-lang/cargo#16225)
@rustbot rustbot added this to the 1.93.0 milestone Nov 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inconsistent panic during cargo vendor

3 participants